-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Revert "[Clang] improve -Wstring-concatenation to warn on every missing comma in initializer lists" #154369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ng comma…" This reverts commit c2eb895.
|
@llvm/pr-subscribers-clang Author: Oleksandr T. (a-tarasyuk) ChangesReverts llvm/llvm-project#154018 Full diff: https://github.com/llvm/llvm-project/pull/154369.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8f80b4cd49bd7..8ee1586e72e01 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -184,8 +184,6 @@ Improvements to Clang's diagnostics
an override of a virtual method.
- Fixed fix-it hint for fold expressions. Clang now correctly places the suggested right
parenthesis when diagnosing malformed fold expressions. (#GH151787)
-- ``-Wstring-concatenation`` now diagnoses every missing comma in an initializer list,
- rather than stopping after the first. (#GH153745)
- Fixed an issue where emitted format-signedness diagnostics were not associated with an appropriate
diagnostic id. Besides being incorrect from an API standpoint, this was user visible, e.g.:
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 5e62c0cf01510..b37a3ffe24f72 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -14708,14 +14708,7 @@ void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
isa<InitListExpr>(var->getInit())) {
const auto *ILE = cast<InitListExpr>(var->getInit());
unsigned NumInits = ILE->getNumInits();
- if (NumInits > 2) {
- auto concatenatedPartsAt = [&](unsigned Index) -> unsigned {
- if (const Expr *E = ILE->getInit(Index))
- if (const auto *S = dyn_cast<StringLiteral>(E->IgnoreImpCasts()))
- return S->getNumConcatenated();
- return 0;
- };
-
+ if (NumInits > 2)
for (unsigned I = 0; I < NumInits; ++I) {
const auto *Init = ILE->getInit(I);
if (!Init)
@@ -14728,23 +14721,24 @@ void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
// Diagnose missing comma in string array initialization.
// Do not warn when all the elements in the initializer are concatenated
// together. Do not warn for macros too.
- if (NumConcat == 2) {
- if (SL->getBeginLoc().isMacroID())
- continue;
-
- unsigned L = I > 0 ? concatenatedPartsAt(I - 1) : 0;
- unsigned R = I + 1 < NumInits ? concatenatedPartsAt(I + 1) : 0;
-
- // Skip neighbors with multi-part concatenations.
- if (R > 1)
- continue;
+ if (NumConcat == 2 && !SL->getBeginLoc().isMacroID()) {
+ bool OnlyOneMissingComma = true;
+ for (unsigned J = I + 1; J < NumInits; ++J) {
+ const auto *Init = ILE->getInit(J);
+ if (!Init)
+ break;
+ const auto *SLJ = dyn_cast<StringLiteral>(Init->IgnoreImpCasts());
+ if (!SLJ || SLJ->getNumConcatenated() > 1) {
+ OnlyOneMissingComma = false;
+ break;
+ }
+ }
- // Diagnose when at least one neighbor is a single literal.
- if (R == 1 || L == 1) {
+ if (OnlyOneMissingComma) {
SmallVector<FixItHint, 1> Hints;
- // Insert a comma between the two tokens of this element.
- Hints.push_back(FixItHint::CreateInsertion(
- PP.getLocForEndOfToken(SL->getStrTokenLoc(0)), ", "));
+ for (unsigned i = 0; i < NumConcat - 1; ++i)
+ Hints.push_back(FixItHint::CreateInsertion(
+ PP.getLocForEndOfToken(SL->getStrTokenLoc(i)), ","));
Diag(SL->getStrTokenLoc(1),
diag::warn_concatenated_literal_array_init)
@@ -14752,9 +14746,10 @@ void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
Diag(SL->getBeginLoc(),
diag::note_concatenated_string_literal_silence);
}
+ // In any case, stop now.
+ break;
}
}
- }
}
diff --git a/clang/test/Sema/string-concat.c b/clang/test/Sema/string-concat.c
index 4b52a74116b49..63abf100c020f 100644
--- a/clang/test/Sema/string-concat.c
+++ b/clang/test/Sema/string-concat.c
@@ -168,31 +168,3 @@ const char *extra_parens_to_suppress_warning[] = {
"promise"),
"shared_future"
};
-
-const char *multiple_missing_commas1[] = {
- "1",
- "2" // expected-note {{place parentheses around the string literal to silence warning}}
- "3", // expected-warning {{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
- "4",
- "5",
- "6" // expected-note {{place parentheses around the string literal to silence warning}}
- "7", // expected-warning {{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
- "8",
- "9",
- "10",
- "11",
-};
-
-const char *multiple_missing_commas2[] = {
- "1",
- "2"
- "3"
- "4"
- "5",
- "6" // expected-note {{place parentheses around the string literal to silence warning}}
- "7", // expected-warning {{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
- "8",
- "9",
- "10",
- "11",
-};
|
AaronBallman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Though it would be helpful to add some information about why it's being reverted to the PR summary/commit message.
|
@AaronBallman I've updated the PR description. |
AaronBallman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
Revert #154018 changes due to excessive false positives. The warning caused multiple benign reports in large codebases (e.g. Linux kernel, Fuchsia, tcpdump). Since many of these concatenations are intentional and follow project style rules, the diagnostic introduced more false positives than value. This will be revisited as a potential
clang-tidycheck instead.